Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve http connection reuse #3760

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Improve http connection reuse #3760

merged 4 commits into from
Jun 20, 2024

Conversation

bmteller
Copy link
Contributor

@bmteller bmteller commented Jun 7, 2024

What this PR does:
Fixes connection reuse for S3 range requests and increases the number of MaxIdleConnsPerHost for the S3 backend to 100 to match the azure settings. Also, assumes GCP has the same connection reuse problem for range requests and performs the same fix. Note: I have tested the fix for S3 range requests and verified it is a problem and the change fixes the problem but I have not verified that it is a problem for GCP or that it fixes the problem for GCP. The GCP HTTP google storage code does look like it effectively returns the body reader so it does look like GCP would have a similar issue.

Which issue(s) this PR fixes:
Fixes #3750

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Jun 7, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

bmteller added 3 commits June 7, 2024 18:55
@bmteller
Fix HTTP Connection Reuse for S3 range requests
cbd16b2 
If EOF is not read then the connection is not eligble for reuse.
Try to read an extra byte to ensure EOF is read.
@bmteller
Explicitly set MaxIdleConnsPerHost/MaxIdleConns for S3 backend
5b5e5ee 
The previous value was the MinIO default which is 16 per
host and 100 global.
@bmteller
Fix HTTP Connection Reuse for GCS range requests
7d00c3d 
If EOF is not read then the connection is not eligble for reuse.
Try to read an extra byte to ensure EOF is read.
@bmteller bmteller force-pushed the improve_http_connection_reuse branch from be7a0ca to 7d00c3d Compare June 7, 2024 17:55
joe-elliott
joe-elliott reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

  • Do you think we should put something similar in Azure?
  • Can you add a changelog entry detailing the fix

@bmteller
Copy link
Contributor Author

I don't think azure has a similar problem unless it is in the azure library itself.

        if _, err := blobClient.DownloadBuffer(ctx, destBuffer, &blob.DownloadBufferOptions{
                Range: blob.HTTPRange{
                        Offset: offset,
                        Count:  size,
                },
                BlockSize:   blob.DefaultDownloadBlockSize,
                Concurrency: maxParallelism,
                RetryReaderOptionsPerBlock: blob.RetryReaderOptions{
                        MaxRetries: maxRetries,
                },

and this blobClient is from github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob.

@bmteller
Copy link
Contributor Author

what should it be classified in the changelog as? BUGFIX?

@joe-elliott
Copy link
Member

what should it be classified in the changelog as? BUGFIX?

Yeah, I think that's fair.

@joe-elliott
changelog
e53d9f8 
Signed-off-by: Joe Elliott <[email protected]>
joe-elliott
joe-elliott approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and made the changelog entry b/c of how valuable this fix is.

Thanks!

@joe-elliott joe-elliott merged commit a210d3d into grafana:main Jun 20, 2024
14 checks passed
@bmteller
Copy link
Contributor Author

thanks Joe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-elliott joe-elliott joe-elliott approved these changes

@annanay25 annanay25 Awaiting requested review from annanay25

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

@mapno mapno Awaiting requested review from mapno mapno is a code owner

@yvrhdn yvrhdn Awaiting requested review from yvrhdn yvrhdn is a code owner

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

@electron0zero electron0zero Awaiting requested review from electron0zero electron0zero is a code owner

@ie-pham ie-pham Awaiting requested review from ie-pham ie-pham is a code owner

@stoewer stoewer Awaiting requested review from stoewer stoewer is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

S3 Not Reusing Connections
3 participants
@bmteller @CLAassistant @joe-elliott